logging: Add compile time error on 64 bit platforms #11110
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11110 +/- ##
==========================================
+ Coverage 52.96% 53.1% +0.13%
==========================================
Files 218 218
Lines 26840 26853 +13
Branches 5949 5950 +1
==========================================
+ Hits 14217 14259 +42
+ Misses 10187 10162 -25
+ Partials 2436 2432 -4
Continue to review full report at Codecov.
|
include/logging/log_core.h
Outdated
| @@ -15,6 +15,10 @@ | |||
| extern "C" { | |||
| #endif | |||
|
|
|||
| #ifdef __LP64__ | |||
There was a problem hiding this comment.
In #9522 the pointers are 32-bit even though the system is running in 64 bit. Would this check work with that PR?
There was a problem hiding this comment.
LP64 by definition, indicates that longs and pointers are 64-bit.
http://www.unix.org/version2/whatsnew/lp64_wp.html
I haven't tested this, but since the x86_64 port runs with 32-bit pointers, I imagine we should be fine, the compiler shouldn't be defining this macro.
There was a problem hiding this comment.
Yeah, x86_64 won't hit this because it's running in the x32 ABI. But... I mean, it's x32 until it's not. A very reasonable future would have us running in a fully 64 bit mode on platforms like x86 and RISC-V. Not a reason to reject this patch if there are known word size issues, but we'll eventually want to fix the underlying issue.
|
I'd prefer to see this done in terms of a sizeof() comparison over the types you actually care about instead of relying on the toolchain to correctly set LP64. We have a lot of oddball toolchains, and I bet somewhere in the world there's a "64 bit" compiler that doesn't set this. |
Logger is designed with assumption that address fit in 32 bits. Added explicit compilation error on 64 bit platforms. Signed-off-by: Krzysztof Chruscinski <[email protected]>
nordic-krch
force-pushed
the
log_error_on_64bit
branch
from
5d67a0e to
91d8d52
Compare
|
changed to |
| @@ -15,6 +15,10 @@ | |||
| extern "C" { | |||
| #endif | |||
|
|
|||
| #if UINTPTR_MAX == 0xFFFFFFFFFFFFFFFFUL | |||
There was a problem hiding this comment.
We also have BUILD_ASSERT_MSG() macro that could be used here, so something like this
BUILD_ASSERT_MSG(sizeof(void *) == 4, "Logger does not support 64 bit architecture");
You seem to be using it already in log_msg.h. I have no preference for this so this proposal is ok too.
|
@andyross can you take another look now that this uses |
Logger is designed with assumption that address fit in 32 bits.
Added explicit compilation error on 64 bit platforms.
Signed-off-by: Krzysztof Chruscinski [email protected]